gh-118150: difflib: expose autojunk flag from SequenceMatcher to public methods and functions - #153959
gh-118150: difflib: expose autojunk flag from SequenceMatcher to public methods and functions#153959faithlesstomas wants to merge 10 commits into
Conversation
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Quick check if this works for the original issue test: #!/usr/bin/python3
import difflib
def get_lines(filename):
with open(filename, 'r', encoding='utf8') as fd:
return fd.readlines()
for autojunk in (True, False):
old_new = list(difflib.unified_diff(
get_lines('small.external.old.json'),
get_lines('small.external.new.json'),
autojunk=autojunk
))
new_old = list(difflib.unified_diff(
get_lines('small.external.new.json'),
get_lines('small.external.old.json'),
autojunk=autojunk
))
print('Autojunk flag:', autojunk)
print('diff external.old external.new.json | wc -l')
print(len(old_new))
print('diff external.new external.old.json | wc -l')
print(len(new_old)) |
|
I think the Documentation ( |
There was a problem hiding this comment.
@faithlesstomas you should review this file, it seems to have been very badly AI-generated.
There was a problem hiding this comment.
@Lenormju I wouldn't have thought that my bad English could be considered to be AI generated :) Do you have any specific suggestion what should be improved in this description?
BTW, nothing in this PR was AI generated.
There was a problem hiding this comment.
@faithlesstomas The commit was titled difflib - fix NEWS issue with ref. not found which I found strange that a human would commit "ref. not found". Also the NEWS file contain these comments :
# # Uncomment one of these "section:" lines to specify which section # this
entry should go in in Misc/NEWS.d. # #.. section: Security #.. section: Core
and Builtins #.. section: Library #.. section: Documentation #.. section:
Tests #.. section: Build #.. section: Windows #.. section: macOS #..
section: IDLE #.. section: Tools/Demos #.. section: C API
# Write your Misc/NEWS.d entry below. It should be a simple ReST paragraph.
# Don't start with "- Issue #<n>: " or "- gh-issue-<n>: " or that sort of
stuff.
###########################################################################
Which look like a template you did not erase.
I was not basing myself on the English, instead on all the rest. But if it wasn't, sorry.
There was a problem hiding this comment.
@Lenormju Sorry, that was my mistake - I thought this comment is to be left there. Also I didn't know how properly reference a class in ReST... But besides that, does this PR look good now?
There was a problem hiding this comment.
The blurb CLI uses .. section: Library to decide which directory to put the news file, in this case it's already in Misc/NEWS.d/next/Library/ as expected.
Please remove .. section: Library and dedent the next paragraph. News files are usually fairly short, often just a line or two, so we could do the same here.
See some other examples in https://github.com/python/cpython/tree/main/Misc/NEWS.d/next/Library
Please also add a What's New entry to https://github.com/python/cpython/blob/main/Doc/whatsnew/3.16.rst, this can be a bit longer if needed.
hugovk
left a comment
There was a problem hiding this comment.
Please could you add tests to Lib/test/test_difflib.py?
| build-ubuntu-ssltests, | ||
| test-hypothesis, | ||
| cifuzz, | ||
| cifuzz |
There was a problem hiding this comment.
What's this change for?
| cifuzz | |
| cifuzz, |
There was a problem hiding this comment.
That was a docs failure, not a CI config failure: https://github.com/python/cpython/actions/runs/29916899467/job/88912837830
And bd98298 didn't fix it, af14a4f did.
Please undo this change.
There was a problem hiding this comment.
The blurb CLI uses .. section: Library to decide which directory to put the news file, in this case it's already in Misc/NEWS.d/next/Library/ as expected.
Please remove .. section: Library and dedent the next paragraph. News files are usually fairly short, often just a line or two, so we could do the same here.
See some other examples in https://github.com/python/cpython/tree/main/Misc/NEWS.d/next/Library
Please also add a What's New entry to https://github.com/python/cpython/blob/main/Doc/whatsnew/3.16.rst, this can be a bit longer if needed.
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
| + emu | ||
| """ | ||
| return Differ(linejunk, charjunk).compare(a, b) | ||
| return Differ(linejunk, charjunk, autojunk).compare(a, b) |
There was a problem hiding this comment.
This is keyword-only.
| return Differ(linejunk, charjunk, autojunk).compare(a, b) | |
| return Differ(linejunk, charjunk, autojunk=autojunk).compare(a, b) |
Until now difflib methods and functions took SequenceMatcher class kwarg autojunk by default which is set up to be True in this class.. This PR aims to expose this flag to the public methods and functions of this module, in order that user can choose behavior of this option in SequenceMatcher.
Issue: #118150
Related PR: #153892